Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move gitlab_project_push_rules to gitlab_project.push_rules #422

Merged
merged 1 commit into from
Sep 21, 2020
Merged

Move gitlab_project_push_rules to gitlab_project.push_rules #422

merged 1 commit into from
Sep 21, 2020

Conversation

armsnyder
Copy link
Collaborator

@armsnyder armsnyder commented Sep 19, 2020 •
edited
Loading

Supersedes #416 #407 #269 #246
Fixes #352

See the discussion in #416 for some background.

The gitlab_project_push_rules resource has caused a lot of confusion because in GitLab EE, push rules are created implicitly when projects are created. This created a situation where if you wanted to use gitlab_project_push_rules you would always have to import it, and you could not create a project with push rules in the same terraform apply.

The solution in the PR is to remove the gitlab_project_push_rules resource, and add a push_rules attribute to the gitlab_project resource and data source.

I took care to add tests for adding, updating, and removing push rules in both EE and CE.

Note that push_rules is Optional+Computed, meaning that if the user sets one of the arguments, then removes the block entirely, it will not be removed from the upstream GitLab.

@armsnyder armsnyder added the breaking change Issue or Pull Request which would introduce a breaking change label Sep 19, 2020
roidelapluie
roidelapluie reviewed Sep 19, 2020
View reviewed changes
docs/data-sources/project.md Outdated
* `packages_enabled` - Enable packages repository for the project.

* `push_rules` Project push rules (GitLab Starter/Bronze/EE only). Push Rules documented below.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we just say EE maybe? We don't give that level of details elsewhere IIRC.

Also can we link to upstream doc?

Copy link
Collaborator Author

@armsnyder armsnyder Sep 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I linked to the upstream doc in the resource doc, where push_rules is an argument. This one is the data source doc. Do you still want me to add it here as well?

I searched our docs for "EE" and the only other instance of it I found was in service_github

requires either EE (self-hosted) or Silver and above (GitLab.com).

So in that case it's equally specific. I'm not sure if there are other EE features in the provider that we simply do not mention are EE only? In that case I can remove the mention of EE in the doc.

@nicholasklick You mentioned people were asking for EE features in this provider. Do you think it makes sense for us to document which fields are EE-only, or can we leave that out for simplicity?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my latest push I removed mention of "EE" in the docs change.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also added the doc link to both the data source and the resource. 😉

docs/resources/project.md Outdated
@@ -80,6 +98,34 @@ The following arguments are supported:

* `packages_enabled` - (Optional) Enable packages repository for the project.

* `push_rules` (Optional) Manage push rules for your GitLab project.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicated?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope, this page is the resource. The earlier one was the data source.

gitlab/resource_gitlab_project.go Outdated
@@ -11,6 +13,8 @@ import (
gitlab "github.com/xanzy/go-gitlab"
)

var errProjectPushRulesUnsupported = errors.New("Project push rules are not supported in your version of GitLab")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
var errProjectPushRulesUnsupported = errors.New("Project push rules are not supported in your version of GitLab")
var errProjectPushRulesUnsupported = errors.New("project push rules are not supported in your version of GitLab")

Copy link
Collaborator Author

@armsnyder armsnyder Sep 19, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In other providers, and in the Hashicorp docs, I have seen them use capitalized errors for errors that are returned from the CRUD functions, because they are user-facing. I moved this error back to inline to ease the confusion. Let me know if you still want me to change it.

gitlab/resource_gitlab_project.go Outdated Show resolved Hide resolved
@armsnyder armsnyder force-pushed the project-push-rules branch 2 times, most recently from ad55365 to e751a05 Compare September 19, 2020 21:28
@roidelapluie
Copy link
Collaborator

an we document like this:

https://registry.terraform.io/providers/hashicorp/kubernetes/latest/docs/resources/api_service

with "Nested blocks"

@armsnyder
Copy link
Collaborator Author

@roidelapluie So you're aware, I changed push_rules to be Optional+Computed. This allowed me to remove the DiffSuppressFunc. It also fixed a minor warning in the TF debug logs when using push_rules:

2020/09/19 12:23:26 [WARN] Provider "registry.terraform.io/-/gitlab" produced an unexpected new value for gitlab_project.this, but we are tolerating it because it is using the legacy plugin SDK.
    The following problems may be the cause of any confusing errors from downstream operations:
      - .push_rules: block count changed from 0 to 1

The side effect of O+C is if a user removes their push_rules block it will not remove the push rules from upstream. However I found multiple examples of O+C on nested blocks in the aws-terraform-provider.

@armsnyder armsnyder force-pushed the project-push-rules branch 3 times, most recently from fb1e65a to 5236721 Compare September 19, 2020 22:47
roidelapluie
roidelapluie approved these changes Sep 21, 2020
View reviewed changes
Copy link
Collaborator

@roidelapluie roidelapluie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome!

@armsnyder armsnyder merged commit 17569db into master Sep 21, 2020
@armsnyder armsnyder deleted the project-push-rules branch September 21, 2020 09:33
This was referenced Sep 21, 2020
Closed
Closed
Closed
Closed
@github-actions github-actions bot locked and limited conversation to collaborators Nov 10, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@roidelapluie roidelapluie roidelapluie approved these changes

Assignees
No one assigned
Labels
breaking change Issue or Pull Request which would introduce a breaking change
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

422 {error: Project push rule exists}
2 participants
@armsnyder @roidelapluie